Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attachment default_url should allow nil #55

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

johnnyshields
Copy link

If I set Paperclip::Attachment.default_options[:default_url] = nil (instead of missing.png) Paperclip gives an error. This PR allows setting to nil, without changing any existing default behavior.

johnnyshields and others added 3 commits March 29, 2021 13:50
If I set `Paperclip::Attachment.default_options[:default_url] = nil` (instead of `missing.png`) Paperclip gives an error. This PR allows setting to nil, without changing any existing behavior.
@ssinghi ssinghi requested a review from sbhawsingka May 27, 2021 17:21
@ssinghi ssinghi self-assigned this May 27, 2021
@sbhawsingka
Copy link
Collaborator

Hi @johnnyshields, thanks for the PR!

Your fix looks good, however, one small concern

image_tag user.avatar.url(:medium)

will lead to the following error - Nil location provided. Can't build URI.

So this change will hamper the ability to follow the common pattern of using the image_tag as shown in the above example.

If the purpose of this PR is only to render nothing in place of a missing image, maybe, we should avoid returning nil from the library so that the common code pattern shown above works safely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants